-
Notifications
You must be signed in to change notification settings - Fork 1
fix F401 E402, E722, and python2 deprecation issues #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
See inline comments. Another good habit to get into when putting in PRs is to review them yourself. I typically do this by going through the code I edited, and if its not super obvious why you made the changes I leave a comment describing what I did. For example, when removing the unused import statements, I might leave an inline comment that says "removing unused imports". Does that make sense? |
| from pyface.api import ImageResource, SplashScreen | ||
| from pyface.api import ImageResource | ||
| from traits.api import ( | ||
| Any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete imports that unused
| from dpx.srxplanargui.srxconfig import SrXconfig | ||
|
|
||
| ETSConfig.toolkit = "qt" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change deprecated "qt4" to "qt"
| if sys.platform == "win32": | ||
| pyFAIdir = os.path.join(sys.exec_prefix, "Scripts") | ||
| elif sys.platform == "linux2": | ||
| elif sys.platform.startswith("linux"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change deprecated "linux2" platform check to "linux"
|
|
||
| def callPyFAICalibration(self, image=None, dspacefile=None): | ||
| if image == None: | ||
| if image is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python2 cond check fix
| temp = re.findall("[-+]?\d*\.\d+|[-+]?\d+", line) | ||
| return map(float, temp) | ||
| """Extract all floats from a string and return them as a list.""" | ||
| pattern = r"[-+]?\d*\.\d+|[-+]?\d+" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python2 formatting issue fix
|
Nice, please see my comments too |
Just to make sure did you make the comments in code review space? I could not see the comments. |
dpx/srxplanargui/help.py
Outdated
|
|
||
| def cpReftext(self): | ||
| cpToClipboard(self.reftext) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be indented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is supposed to be indented, as it has self.reftext, thenthereftext` str variable should be indented. I have made that change.
dpx/srxplanargui/calibration.py
Outdated
| show_border=True, | ||
| label="Plasee specify the dimension of detector and size of pixel:", | ||
| label="Plasee specify the dimension of detector" | ||
| "and size of pixel:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this syntax valid? do you need a + or to wrap it in parenths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in recent push
dpx/srxplanargui/calibration.py
Outdated
| ), | ||
| label="Please specify the wavelength and distance between sample and detector:", | ||
| label="Please specify the wavelength and" | ||
| "distance between sample and detector:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved in recent push
|
oh true, thanks. you should be able to see it now |
|
@cadenmyers13 All the comments are edited, please see the new version and ready for review |
cadenmyers13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment
dpx/srxplanargui/calibration.py
Outdated
| show_border=True, | ||
| label="Plasee specify the dimension of detector and size of pixel:", | ||
| label="Plasee specify the dimension of detector" | ||
| + "and size of pixel:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you add a space here and to other similar areas (i.e. + " and... instead of + "and...)
|
@cadenmyers13 Have changed for label, ready for review. |
|
@stevenhua0320 Thanks. @sbillinge ready for review by you |
|
@stevenhua0320 @cadenmyers13 This PR seems to be into the |
|
@sbillinge Because |
|
Closed #8 as not merge to right branch |



@cadenmyers13 Ready for review